Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Autofocus confirm password textfield on password field submission. #1288

Closed
wants to merge 1 commit into from
Closed

fix: Autofocus confirm password textfield on password field submission. #1288

wants to merge 1 commit into from

Conversation

CyberWake
Copy link

What

  • This PR fixes the auto focus of confirm password text field on submission of password field.

Screenshot

Before After

Fixes bug(s)

@CyberWake CyberWake requested a review from a team as a code owner March 21, 2022 17:47
@teolemon teolemon added the 👥 User management Account login, signup, signout label Mar 21, 2022
g123k
g123k reviewed Mar 22, 2022
View reviewed changes
Copy link
Collaborator

@g123k g123k left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @CyberWake.
However I think a generic solution would be better here.

Instead of doing the focus trick on the password field only, it would be better to:

  • Add a nextFocus optional field to SmoothTextFormField
  • If it's provided, do your call in the onFieldSubmitted

M123-dev
M123-dev reviewed Mar 22, 2022
View reviewed changes
packages/smooth_app/lib/generic_lib/widgets/smooth_text_form_field.dart Show resolved Hide resolved
packages/smooth_app/lib/pages/user_management/sign_up_page.dart
),
const SizedBox(height: space),
SmoothTextFormField(
focusNode: confirmPassword,
type: TextFieldTypes.PASSWORD,
controller: _password2Controller,
textInputAction: TextInputAction.next,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure your way also works, but have you tried setting the textInputAction for the last field to TextInputAction.done, besides of that I can't think of why the focus move doesn't work for the last one

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So setting the textInputAction to TextInputAction.done for the confirm password field won't work I guess since the issue starts with the internal mechanism of flutter I guess since the control doesn't get passed automatically ideally the focus passes to the next field inside the for without use of focusNode or textInputAction. But sometimes this doesn't happen due to some reasons like use of obscure field etc. Then in that case we explicitly need to use this focusNode mechanism.

@CyberWake
Copy link
Author

Thanks for your PR @CyberWake. However I think a generic solution would be better here.

Instead of doing the focus trick on the password field only, it would be better to:

  • Add a nextFocus optional field to SmoothTextFormField
  • If it's provided, do your call in the onFieldSubmitted

Surely I try this one since this reduces passing of one argument. Good suggestion hope this works. 🤞🏻

@CyberWake
Copy link
Author

I have an input from my side to stand the current implementation. Like in my case I have added the condition if the confirm password is not same as password then only focus to confirm password field this custom logic cannot be added if we implement that common way of using nextFocus. What I mean to say is like in my methodology if the user fills in the email field suppose first then he fills in the name field now via my mechanism we can directly add this logic to move the focus to directly to username field reducing user clicks. With nextFocus mechanism he would have to first submit name field then email field which was already filled and then again he needs to submit the email field to reach username field.

Secondly in nextFocus methodology we'll still need to pass the focus for the current field like for confirm password SmoothTextFormField we'll need to pass the focus to uniquely identify the confirm password fields focus. So the reduction of argument won't we a case which I just got.

Let me know you input on this.

@CyberWake
Copy link
Author

@M123-dev could you please update me on this PR.

@g123k
Copy link
Collaborator

g123k commented Mar 24, 2022

I guess you can my solution can easily be implemented.
Actually, you have two arguments:

  • A nextFocus, to allow the call of FocusScope.of(...)
  • A callback of type bool Function(String?), where you express if the call to nextFocus should be done or not

@CyberWake
Copy link
Author

@g123k the solution suggested still need the two argument one the focusNode of the current field and the nextFocus which you want the focus to move after submit. As the nextFocus stand alone cannot move the focus to next textField since the value of nextFocus needs to be assigned as a focusNode to some textfield.

@M123-dev M123-dev requested a review from g123k March 28, 2022 06:19
@teolemon teolemon linked an issue Mar 28, 2022 that may be closed by this pull request
Bug: Confirm password field doesn't come to focus on submission of Password field unlike other fields(Signup Page) #1286
Closed
@M123-dev M123-dev mentioned this pull request Mar 30, 2022
Merged
@teolemon teolemon added this to the V 1 milestone Mar 31, 2022
M123-dev
M123-dev reviewed Apr 1, 2022
View reviewed changes
packages/smooth_app/lib/pages/user_management/sign_up_page.dart
Comment on lines +136 to +140
// Checks if confirm password is equal to password if not then
// move the focus to the confirm password field
if (_password2Controller.text != value) {
FocusScope.of(context).requestFocus(confirmPassword);
}
Copy link
Member

@M123-dev M123-dev Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then lets leave it like that, just two small things to sum up

  1. I could imagine it beeing a bit confusing if sometimes you jump to the second password and another time two steps further. Its certainly great when you know it, but when not not
  2. Please add a comment here and where you create the Focus node that there was a bug and thats why we have this hacky way for changing the focus only for one field

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M123-dev sure I'll do the points marked out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update @CyberWake

@teolemon
Copy link
Member

teolemon commented Jul 8, 2022

merge conflicts on this one

@g123k
Copy link
Collaborator

g123k commented Jul 8, 2022

I think we should work on #2555 and dismiss this PR

@teolemon
Copy link
Member

teolemon commented Jul 8, 2022

ok, closing 👍
sorry for the inconvenience @CyberWake

@teolemon teolemon closed this Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M123-dev M123-dev M123-dev left review comments

@g123k g123k Awaiting requested review from g123k

Assignees

@CyberWake CyberWake

Labels
merge conflicts 🎯 P1 PR: needs rebase 👥 User management Account login, signup, signout
Projects
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Archived in project
Milestone
V1
4 participants
@CyberWake @g123k @teolemon @M123-dev